Skip to content

Conversation

@perlinm
Copy link
Contributor

@perlinm perlinm commented Dec 9, 2024

fixes #95

In my view this fix is a bit of a hack around peculiarities of the Permute instruction, which (at least in its current implementation) is fundamentally inconsistent with broadcasting instructions across a qubit register (see comment). The fix in this PR is to just unroll broadcast operations, and make them act on the individual qubits of a register.

I'm happy to fix the broken tests if+when the solution in this PR is approved.

If I were to opine a bit: in my view Permute looks to be a re-invention of the SWAP gate, and it would be much more favorable to just use SWAPs instead of Permutes, and in turn have hardware recognize SWAPs as virtual (handled by some below-QASM level of control software, essentially mutating the map from qubit label --> physical qubit/ion). Inserting a single SWAP/Permute into a circuit shouldn't have the effect of (a) modifying all downstream QASM and (b) making visual inspection and debugging of QASM impossible, because the QASM no longer agrees with the SLR it was produced from (asking SLR to pecos.qeclib.qubit.X(qreg_a[0]) can now produce the QASM x qreg_b[2], for example). And of course broadcasting instructions across a register is a nice and natural capability to have. Having said that, we still need Permute for now, so hopefully this is an acceptable fix 🙂

@perlinm perlinm requested a review from ciaranra as a code owner December 9, 2024 15:30
Comment on lines 302 to 310
elif isinstance(q, tuple):
if len(q) != op.qsize:
msg = f"Expected size {op.qsize} got size {len(q)}"
raise Exception(msg)
qs = ",".join([str(qi) for qi in q])
str_list.append(f"{repr_str} {qs};")

str_list.append(f"{repr_str} {str(q)};")
else:
str_list.append(f"{repr_str} {q};")
Copy link
Contributor Author

@perlinm perlinm Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me like that the unconditional final str_list.append(...) was a bug, and should have been inside an else statement. Otherwise this method first acts the gate on every qs in q and then again appends a gate on (the tuple) q.

Copy link
Member

@ciaranra ciaranra Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, SWAP is a unitary gate that could possibly be converted to a physical permute if a compiler is smart enough. There is a need to distinguish the two operations as you may what fine control and choose one over the other and do not want to rely on a compiler.

As far as rewriting QASM, the user should no longer be responsible with debugging that. This is the job of the language (e.g., SLR)/compiler. If the underlying base language had support for Permute, then that would make things cleaner; however, SLR needs to metaprogram features that are not supported by the base language which is essentially what higher languages have to do when converting to lower languages. They provide more facilities then the simpler underlying language provides.

That being said. I want to rewrite the Permute object to be a little more friendly to code generation and just stores the info in a nice manner to be handed off to the code generator function.

Copy link
Contributor Author

@perlinm perlinm Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upgrading SWAP to a "native gate" of the hardware is perfectly compatible with thinking of it as a unitary gate. Relabeling qubits does implement the desired unitary operation, and it is an advantage of trapped ion platform that it can perform SWAP gates perfectly 🙂! Not all platforms can do this. This is also a capability that can be leveraged by circuit-level compilers (search this paper for "swap mirroring", for example), but doing so requires that the hardware recognize SWAP as a native gate. This essentially "exposes" the virtual SWAP capability to compilers that work on the level of (say) pytket, cirq, or qiskit.

There is also precedent here: having a virtual native SWAP gate is directly analogous to virtual Rz gates on IBM devices. IBM devices accept Rz instructions, but do not actually apply any physical operations any qubits when asked to "apply" an Rz gate (they instead make a software-level update of a rotating frame).

From the perspective of compiler development, the way to get "fine control" and execute a specific sequence of physical operations is to build the circuit you want, and run it in "verbatim" mode to bypass/disable any compiler. Another option is to have compilation flags that toggle various compiler features. There is no control or choice removed by upgrading the SWAP gate to a native virtual operation (and in fact, at the moment you already "rely on a compiler" to convert a SWAP instruction into 3 MS gates).

Final point:

As far as rewriting QASM, the user should no longer be responsible with debugging that.

As a matter of practice, we have definitely resorted to examining QASM to debug PECOS and would appreciate the option to continue doing so 🙂

In any case, what are your thoughts of unrolling broadcast operations in this PR? Some fix for #95 is necessary to use both Permute and (say) Steane.sz in the same SLR (and Permute is used for Steane.t_tel).

Copy link
Member

@ciaranra ciaranra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ciaranra ciaranra merged commit ce846b8 into PECOS-packages:development Dec 14, 2024
17 checks passed
@perlinm perlinm deleted the unroll-broadcast branch December 14, 2024 18:21
@perlinm
Copy link
Contributor Author

perlinm commented Dec 14, 2024

Thanks for the review and test fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants